Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add simplified service worker invalidation #2551

Merged
merged 2 commits into from
Jun 27, 2017

Conversation

ro-savage
Copy link
Contributor

@ro-savage ro-savage commented Jun 17, 2017

Adds invalidation of service workers based on by attempting to fetch service-worker.js and if its receives a 404, or if its the wrong content type (e.g. all not found urls are being redirected to index.html or serving a 404 page without a 404 code) it unregistered the current service worker and reloads the page.

This issues the issue where sites hosted on the same url:port (e.g. localhost:5000) are incorrectly displayingcreate-react-app instead of the hosted page.

This was addressed in a more complex manner in #2438. However, this is a simplified version based on the feedback of @jeffposnick.

Note: I created a new PR/Branch because I believe #2438 addresses more cases, however it also adds complexity. This PR should fix most use cases when combined with #2426.

@ro-savage
Copy link
Contributor Author

@gaearon @tizmagik
This is ready for review and merge. Should not cause any breaking changes.

@jeffposnick you might want to double check, its based on your feedback.

CI failing is unrelated to change. It is some issue with windows build and the latest version of appveyor.

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the caveat that it's unfortunate that this sort of logic needs to be implemented by each framework/starter kit, rather than being baked in to the browser, I understand why you'd want to move ahead with this brute-force approach in the interim.

I've just got the one comment about potentially only running this additional logic on localhost instead of in production as well.

};
};

fetch(swUrl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing this out there: the overhead of this extra request would be reduced if there were an additional check that only caused it to execute when you're on localhost or the equivalent. This PR would then solve the "zombie" SW on localhost problem immediately, but would not attempt to unregister a service worker on a production server.

// Inside the window.onload handler:

const isLocalhost = Boolean(window.location.hostname === 'localhost' ||
  // [::1] is the IPv6 localhost address.
  window.location.hostname === '[::1]' ||
  // 127.0.0.1/8 is considered localhost for IPv4.
  window.location.hostname.match(
    /^127(?:\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}$/
  )
);

if (isLocalhost) {
  fetch(swUrl) // ...the rest of your logic here...
} else {
  registerValidSW(swUrl); // Avoid the extra fetch() outside of localhost.
}

It's unclear how w3c/ServiceWorker#204 will get resolved, but there has been some opposition to automatically unregistering production servers on an HTTP 404 response. My hope is that the code that comes out of this PR will evolve (post-merge) to match whatever's decided upon in that issue, providing equivalent behavior prior to the actual native browser implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jeff!

@gaearon
Copy link
Contributor

gaearon commented Jun 26, 2017

@ro-savage

Do you mind scoping this for localhost for now, as suggested by @jeffposnick?

@gaearon gaearon added this to the 1.0.x milestone Jun 26, 2017
@ro-savage
Copy link
Contributor Author

Added the check so it only applies on local host as per @jeffposnick code.*

Personally, I liked the check happening on live servers because it means that if you deployed an app with CRA and then later deployed an app with some other boilerplate, to the same domain. The user (and developer) doesn't get stuck.

It also shouldn't cause a slow down, because we are instantly displaying the app regardless. Its just if there is no service worker found. We reload it right away with the SW removed. (Alt. would be to removed the SW and show the toast from #2426).

However, this is a good start and would be useful to be merged in with only localhost.

@jeffposnick
Copy link
Contributor

👍

@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

I haven't tested but I trust this works.
However longer term this is not very sustainable. Filed #2638.

@gaearon gaearon merged commit 78dbf7b into facebook:master Jun 27, 2017
@gaearon gaearon mentioned this pull request Jun 28, 2017
@gaearon
Copy link
Contributor

gaearon commented Jun 28, 2017

@jeffposnick

I'd appreciate if you could test the latest version and ensure it works as intended.
https://github.com/facebookincubator/create-react-app/releases/tag/v1.0.8

@jeffposnick
Copy link
Contributor

Will do. I'll report back.

@gaearon
Copy link
Contributor

gaearon commented Jun 28, 2017

Thanks!

@jeffposnick
Copy link
Contributor

LGTM—I tried out both the local service worker invalidation scenario, and the PUBLIC_URL is set to a cross-origin scenario, and the PRs to deal with both seem to be working as expected.

@gaearon
Copy link
Contributor

gaearon commented Jun 28, 2017

Thanks again for checking.

romaindso pushed a commit to romaindso/create-react-app that referenced this pull request Jul 10, 2017
* Add service worker invalidation

* Update valid service worker check only on local host
wmonk referenced this pull request in wmonk/create-react-app-typescript Aug 7, 2017
* Add service worker invalidation

* Update valid service worker check only on local host
morgs32 pushed a commit to BrickworkSoftware/create-react-app that referenced this pull request Sep 1, 2017
* Add service worker invalidation

* Update valid service worker check only on local host
window.location.hostname === 'localhost' ||
// [::1] is the IPv6 localhost address.
window.location.hostname === '[::1]' ||
// 127.0.0.1/8 is considered localhost for IPv4.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/8?

Copy link
Contributor

@Timer Timer Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants